Revert "Rework skill validator script (#11)"#15
Conversation
This reverts commit 7a4ace9.
There was a problem hiding this comment.
Pull request overview
Reworks the CI skill validation approach as part of reverting prior changes from #11, moving validation logic into a GitHub Actions-focused script and updating the workflow to validate only PR-touched skills.
Changes:
- Remove the previous
tools/validate-skills.shvalidator script. - Add a new
.github/scripts/validate-skills.shthat detects changed skill directories viagit diffand validates only those. - Update the GitHub Actions workflow to run validation only for PRs that touch
skills/**, and to invoke the new script.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tools/validate-skills.sh |
Deleted the previous “validate all skills” script. |
CONTRIBUTING.md |
Removed contribution/testing guidance that referenced the old script. |
.github/workflows/validate-skills.yml |
Updates CI to validate changed skills and adds PR path filtering + full fetch. |
.github/scripts/validate-skills.sh |
New CI helper script to compute changed skills and run skill-validator. |
Comments suppressed due to low confidence (1)
.github/workflows/validate-skills.yml:20
- The workflow references
actions/checkout@v6andactions/setup-go@v6. As of the currently published Actions releases, these major versions don’t exist (latest known are checkout v4 and setup-go v5), so the workflow will fail to resolve the actions. Update to the latest existing major versions.
- uses: actions/checkout@v6
with:
# Full history is needed so git diff can compare against the base branch
fetch-depth: 0
- name: Set up Go
uses: actions/setup-go@v6
with:
go-version: stable
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "❌ Skill validation failed!" | ||
| echo "" | ||
| echo "📋 See the Job Summary for detailed validation results:" | ||
| echo " https://github.com/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" |
There was a problem hiding this comment.
The failure message always prints a GitHub Actions run URL using $GITHUB_REPOSITORY and $GITHUB_RUN_ID. When this script is run outside Actions, those env vars will be unset and the URL will be malformed. Consider only printing the URL when GITHUB_ACTIONS is set (or when both variables are non-empty).
| echo " https://github.com/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" | |
| if [ -n "${GITHUB_ACTIONS:-}" ] && [ -n "${GITHUB_REPOSITORY:-}" ] && [ -n "${GITHUB_RUN_ID:-}" ]; then | |
| echo " https://github.com/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" | |
| fi |
| on: | ||
| pull_request: | ||
| paths: | ||
| - "skills/**" |
There was a problem hiding this comment.
With pull_request.paths limited to skills/**, this workflow will not run when only the validation script or workflow definition changes. That can allow CI-breaking changes to merge without being exercised. Consider including .github/workflows/validate-skills.yml and .github/scripts/validate-skills.sh in the paths filter (or removing the filter).
| - "skills/**" | |
| - "skills/**" | |
| - ".github/workflows/validate-skills.yml" | |
| - ".github/scripts/validate-skills.sh" |
| set -uo pipefail | ||
|
|
||
| BASE_REF="${1:-}" | ||
|
|
||
| # Find unique skill directories containing files changed in this PR. | ||
| # The three-dot diff requires fetch-depth: 0 and a properly configured remote, | ||
| # which is always the case on GitHub Actions but may not be in local act runs. | ||
| changed_skills=() | ||
| if [ -n "$BASE_REF" ]; then | ||
| mapfile -t changed_skills < <(git diff --name-only "origin/${BASE_REF}...HEAD" -- skills/ \ | ||
| 2>/dev/null \ | ||
| | cut -d'/' -f2 \ | ||
| | sort -u \ | ||
| | grep -v '^$') |
There was a problem hiding this comment.
This script uses mapfile, which is not available in the default macOS /bin/bash (3.2). Since the shebang is #!/bin/bash and the comments mention local usage (e.g., act runs), running it locally on macOS will fail. Either avoid mapfile (e.g., use a while IFS= read -r loop) or explicitly require Bash >= 4 (and consider #!/usr/bin/env bash).
Reverts #11
There are some changes here that don't match the behaviors I think we need to encode in our CI, so I'd like to revert the PR and discuss/modify your original PR as needed.